Use a less constraining memory order for all "update" operations on Counter/Gauge. #623
Conversation
Romain-Geissler-1A
force-pushed
the
relaxed-atomic-operations
branch
2 times, most recently
from
81f3597 to
8643c02
Compare
…ounter/Gauge. Updates on metrics is something that happens much more often than collecting operations, and it doesn't require strong atomic guarantees (ie if two threads update a metric at the same time, we might accept that it's not always the very last update that is being kept in the metric). We sacrifice a bit correctness in case of multithread concurrent updates (which is a rather rare scenario) for better performances all the time (ie even with a single thread, or not concurrent updates).
Romain-Geissler-1A
force-pushed
the
relaxed-atomic-operations
branch
from
8643c02 to
cfb37a3
Compare
|
Ping |
|
@Romain-Geissler-1A have you done any benchmarks for the effect of your change? |
|
No. We have carried this patch on our side for couple of years, back then we had identified these atomic operations as a bit too costly for the need it serves, and decided to relax a bit the memory barrier implied by the default memory order. It was a rather general update in our code, in multiple places, not only in this particular library. I don't really plan on trying to bench this. At worse case it changes absolutely nothing in terms of perf, and at best it may run a bit faster on some architecture. Also it strongly depends on how much multi-threaded your program is and how many concurrent metrics update you have. I am just trying to upstream a patch we have internally. |
Note: this contains the commit of #622 which you shall review/merge separately from this pull request.